Checkpoint/Restore test fixes by adrianreber · Pull Request #11955 · containers/podman

adrianreber · 2021-10-13T13:35:11Z

Moving to Fedora 35 showed test failures (time outs) in the test

"podman checkpoint and restore container with different port mappings"

The test starts a container and maps the internal port 6379 to the local
port 1234 ('-p 1234:6379') and then tries to connect to localhost:1234

On Fedora 35 this failed and blocked the test because the container was
not yet ready. The test was trying to connect to localhost:1234 but
nothing was running there. So the error was not checkpointing related.

Another problem with this test and running ginkgo in parallel was that
it was possible that the port was already in use. Now for each run a
random port is selected to decrease the chance of collisions.

This tries to fix problems seen in #11795

CC: @cevich

mheon · 2021-10-13T15:18:10Z

@edsantiago PTAL

cevich · 2021-10-13T15:25:52Z

My only concern is that perhaps the 3-second timeout (w/ 5 retries) is too long. However, the code also isn't waiting for a 'ready' signal from the container (can it?), and the net.dial() docs do say this timeout includes DNS (which is "special" in case of a container). So maybe it's okay?

test/e2e/checkpoint_test.go

adrianreber · 2021-10-13T15:53:25Z

Switched to WaitContainerReady().

edsantiago

LGTM, one suggestion.

test/e2e/checkpoint_test.go

cevich · 2021-10-14T13:49:34Z

LGTM

cevich · 2021-10-14T14:38:36Z

/lgtm

cevich · 2021-10-14T14:38:41Z

/hold

cevich · 2021-10-14T15:26:17Z

/hold cancel

jwhonce

Issues I see are mostly modifying a known good port. Those new ports could be used elsewhere which cause CI flakes. Generate all "new" port numbers using GetRandomPort()

jwhonce · 2021-10-14T18:21:43Z

test/e2e/checkpoint_test.go

+		fmt.Fprintf(os.Stderr, "Trying to connect to redis server at localhost:%d", orginalPort)
+		// Open a network connection to the redis server via initial port mapping
+		conn, err := net.DialTimeout("tcp4", fmt.Sprintf("localhost:%d", orginalPort), time.Duration(3)*time.Second)
+		Expect(err).To(BeNil())


Suggested change

Expect(err).To(BeNil())

Expect(err).ShouldNot(HaveOccurred())

jwhonce · 2021-10-14T18:23:07Z

test/e2e/checkpoint_test.go

-		}
+		fmt.Fprintf(os.Stderr, "Trying to reconnect to redis server at localhost:%d", orginalPort+100)
+		conn, err = net.DialTimeout("tcp4", fmt.Sprintf("localhost:%d", orginalPort+100), time.Duration(3)*time.Second)
+		Expect(err).To(BeNil())


Suggested change

Expect(err).To(BeNil())

Expect(err).ShouldNot(HaveOccurred())

jwhonce · 2021-10-14T18:28:35Z

test/e2e/checkpoint_test.go

-		localRunString := getRunString([]string{"-p", "1234:6379", "--rm", redis})
+		port, err := utils.GetRandomPort()
+		Expect(err).ShouldNot(HaveOccurred())
+		orginalPort := port + GinkgoParallelProcess()


I'm confused, you get a random port (that is "safe") and then add something to it?

jwhonce · 2021-10-14T18:29:18Z

test/e2e/checkpoint_test.go

+			Fail("Container failed to get ready")
 		}
+
+		fmt.Fprintf(os.Stderr, "Trying to connect to redis server at localhost:%d", orginalPort)


Debugging information?

I think so, the test was hanging just after this point, so knowing what it was trying to do may also be helpful in the future. Ginkgo doesn't give us this detail otherwise 😕

Exactly. It was not added for debugging just for better feedback during the test to know what is happening.

jwhonce · 2021-10-14T18:29:44Z

test/e2e/checkpoint_test.go


 		// Restore container with different port mapping
-		result = podmanTest.Podman([]string{"container", "restore", "-p", "1235:6379", "-i", fileName})
+		result = podmanTest.Podman([]string{"container", "restore", "-p", fmt.Sprintf("%d:6379", orginalPort+100), "-i", fileName})


Again, adding a number to a "safe" port?

jwhonce · 2021-10-14T18:30:09Z

test/e2e/checkpoint_test.go

 		// Open a network connection to the redis server via initial port mapping
 		// This should fail
-		conn, err = net.Dial("tcp", "localhost:1234")
+		conn, err = net.DialTimeout("tcp4", fmt.Sprintf("localhost:%d", orginalPort), time.Duration(3)*time.Second)


I need to steal this one. :-)

test/e2e/checkpoint_test.go

cevich · 2021-10-14T18:44:16Z

Those new ports could be used elsewhere which cause CI flakes

Oooohhhh, I understand. So the concern is if this test goes south/hangs/leaks, another test might also ask for a port and receive the same one (since it was port+number for this test). Very observant of you, and thanks for mentioning this. Seems like an easy enough fix too.

Moving to Fedora 35 showed test failures (time outs) in the test "podman checkpoint and restore container with different port mappings" The test starts a container and maps the internal port 6379 to the local port 1234 ('-p 1234:6379') and then tries to connect to localhost:1234 On Fedora 35 this failed and blocked the test because the container was not yet ready. The test was trying to connect to localhost:1234 but nothing was running there. So the error was not checkpointing related. Before trying to connect to the container the test is now waiting for the container to be ready. Another problem with this test and running ginkgo in parallel was that it was possible that the port was already in use. Now for each run a random port is selected to decrease the chance of collisions. Signed-off-by: Adrian Reber <areber@redhat.com>

adrianreber · 2021-10-14T18:56:52Z

@jwhonce Thanks for the review. I adapted the PR to fix what you commented on. Besides the debug output which was added to know that the test is trying to connect to the container. I can remove it but I think it can be helpful understanding what is going on.

jwhonce · 2021-10-15T14:53:11Z

/lgtm

openshift-ci · 2021-10-15T14:56:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianreber, cevich, jwhonce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jwhonce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cevich · 2021-10-15T14:57:48Z

Thanks everyone.

cevich reviewed Oct 13, 2021

View reviewed changes

test/e2e/checkpoint_test.go Outdated Show resolved Hide resolved

adrianreber force-pushed the 2021-10-13-f35-checkpoint-test-fix branch from ee739d7 to 40ef16c Compare October 13, 2021 15:52

edsantiago reviewed Oct 13, 2021

View reviewed changes

test/e2e/checkpoint_test.go Outdated Show resolved Hide resolved

cevich mentioned this pull request Oct 14, 2021

Cirrus: Bump Fedora to release 35 #11795

Merged

cevich approved these changes Oct 14, 2021

View reviewed changes

adrianreber force-pushed the 2021-10-13-f35-checkpoint-test-fix branch from 40ef16c to b33845f Compare October 14, 2021 13:59

openshift-ci bot assigned cevich Oct 14, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021

jwhonce suggested changes Oct 14, 2021

View reviewed changes

jwhonce added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2021

adrianreber force-pushed the 2021-10-13-f35-checkpoint-test-fix branch from b33845f to 8439a6d Compare October 14, 2021 18:53

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2021

cevich mentioned this pull request Oct 14, 2021

Test-hang fix: Wait for ready + timeout on connect. #11979

Merged

jwhonce approved these changes Oct 15, 2021

View reviewed changes

jwhonce removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2021

openshift-ci bot assigned jwhonce Oct 15, 2021

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 15, 2021

openshift-merge-robot merged commit 171f7b8 into containers:main Oct 15, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

	Expect(err).To(BeNil())
	Expect(err).ShouldNot(HaveOccurred())

Conversation

adrianreber commented Oct 13, 2021

Uh oh!

mheon commented Oct 13, 2021

Uh oh!

cevich commented Oct 13, 2021

Uh oh!

Uh oh!

adrianreber commented Oct 13, 2021

Uh oh!

edsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cevich commented Oct 14, 2021

Uh oh!

cevich commented Oct 14, 2021

Uh oh!

cevich commented Oct 14, 2021

Uh oh!

cevich commented Oct 14, 2021

Uh oh!

jwhonce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cevich commented Oct 14, 2021

Uh oh!

adrianreber commented Oct 14, 2021

Uh oh!

jwhonce commented Oct 15, 2021

Uh oh!

openshift-ci bot commented Oct 15, 2021

Uh oh!

cevich commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants